chore: fix documentation drift, inaccurate comments, and unit test noise - #1527
chore: fix documentation drift, inaccurate comments, and unit test noise#1527Pangjiping wants to merge 5 commits into
Conversation
…m chart The chart's Pool CRD was missing the status.updated field and the UPDATED/AGE printcolumns present in config/crd/bases, so Helm-installed clusters silently dropped status.updated and showed no UPDATED column. Also fix the ObservedGeneration doc comment (copy-pasted from BatchSandbox) in the type and both generated CRD files.
- sandbox-lifecycle.yml: Endpoint URL format (/port/ -> /proxy/), async
creation semantics (202 + Pending, not synchronous Running), NetworkPolicy
empty-body reset to deny-all, IP/CIDR egress targets now supported
- execd-api.yaml: /command/{id}/logs cursor is a byte offset (file seek), not
a line index; GET /code/contexts language is optional; drop never-emitted
'destroyed' session status; document optional access-token enforcement;
cpu_count reflects GOMAXPROCS
- egress-api.yaml: NetworkPolicy empty body resets to deny-all (was allow-all),
IP/CIDR targets supported in dns+nft mode, document POST/PUT /policy
- docs/api/index.md: add missing isolated/credential-vault/metrics endpoints,
ping SSE event, Resuming state, optional execd auth
- docs/guides/pause-resume.md: align state casing with the spec enum
…guides - server: add missing [tenants] and [ingress].secure_access docs, env vars, api-key exempt routes; fix example config comments (empty allowed_host_paths rejects bind mounts, not allows all); correct DEVELOPMENT.md endpoint formats and function names; fix stale links and TODO comments - execd: fix RELEASE_NOTES image tags (1.0.8-1.0.1 all said v1.0.9), add missing isolation metrics, document HOST_IP metrics export fallback - egress/ingress: fix spec-vs-docs contradictions (IP/CIDR, empty policy, header stripping), document secure-access and POST/PUT /policy, remove bogus DNS cache-hit claims, fix Go version/binary path/org links - kubernetes: correct project structure paths, chart versions, task-executor runtime description, annotation/label contract lists, proposal status - cli: add missing credential-vault command group and bundled skill to README and docs site - sdks: fix Kotlin KDoc usage example (stale API), Python language lists (Kotlin -> Go/TypeScript), MCP config docs, remove deprecated Ports from Go/C# credential examples, fix C# cleanup notes - examples: fix openclaw env vars and entrypoint, desktop VNC_PASSWORD requirement, chrome entrypoint guidance, codex/aio-sandbox descriptions, wire playwright TARGET_URL, drop unused windows port
- server: fix file log default path comment, snapshot 'inline' docstrings (work happens on background threads), sandbox service docstring, remove verbatim 'Delegate to the service layer' comments - execd: fix env blacklist scope comment, WS takeover timeout comment, jupyter typo and noise comments, remove empty registerDefaultHandlers, drop never-produced 'destroyed' status from model comment - egress/ingress: fix Dockerfile pre-start hook comment, mergeKey comment, endpoint list comment, secure-access annotation comment, deprecated header comment duplication - internal: fix delta temporality selector comment (ObservableCounter is cumulative), supervisor backoff jitter/burst threshold wording, event writer comments, add missing safego doc comments, logger config scope - kubernetes: remove kubebuilder scaffold TODOs and empty placeholders, fix Pool/BatchSandbox status comments, 'Host mode' -> Local/Sidecar - cli: remove ghost 'osb sb' alias comment, fix output format resolution docstring, clarify command run output help, add credential-vault skill area - sdks: fix ExecutionHandlers async example, code-interpreter language lists and usage example, sync model symmetry comment, interrupt typo
Strip verbatim line-by-line restatements, duplicated blocks, scaffold placeholders, and copy-paste residue across unit tests; keep all explanation-of-why comments, section markers, and meaningful TODOs. - server/tests (+tests/k8s): ~35 comments - components/execd, egress, ingress, internal: ~180 comments (Go) - kubernetes internal tests: 19 comments - cli/tests: 1 comment - sdks (Go, Kotlin): 2 comments
github-actions
Bot
added
component/egress
component/execd
component/ingress
component/k8s
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8ded0f771
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Sandbox creation accepted and provisioning started asynchronously. | ||
|
|
||
| The returned sandbox includes: | ||
| - `id`: Unique sandbox identifier | ||
| - `status.state: "Running"` (provisioning completed synchronously) | ||
| - `status.state`: one of `Pending` (provisioning in progress) or `Running` (already provisioned) |
There was a problem hiding this comment.
The inspected Docker and Kubernetes create paths do not return asynchronously: Docker awaits the future completed by _provision_sandbox, while Kubernetes awaits _wait_for_sandbox_ready, which returns only for Running/Allocated and normalizes the latter to Running. Consequently, a create response cannot contain the newly documented Pending state, and clients following this contract may incorrectly expect a quick acceptance response instead of allowing for image-pull and readiness latency. Revert this description or align both implementations before publishing asynchronous semantics.
AGENTS.md reference: specs/AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
| status: | ||
| type: string | ||
| enum: ["active", "dead", "destroyed"] | ||
| enum: ["active", "dead"] |
There was a problem hiding this comment.
Narrowing this public enum removes a previously valid contract value, while checked-in generated consumers still expose it in Python's session_state_status.py and JavaScript's execd.ts. The next required regeneration would remove those SDK enum members and can break source compatibility or validation of stored/mock responses using destroyed; retain the value or introduce an explicit deprecation and migration instead of removing it as documentation cleanup.
AGENTS.md reference: specs/AGENTS.md:L57-L63
Useful? React with 👍 / 👎.
| - An empty request body resets the policy to deny-all. | ||
| - An object body is parsed as a `NetworkPolicy`; an empty object or | ||
| `null` also resets the policy to deny-all. | ||
| - `PUT` behaves identically to `POST`. |
There was a problem hiding this comment.
PUT is mentioned only inside the post operation's prose; there is no sibling put: operation under /policy. Although policy_server.go accepts http.MethodPut, OpenAPI-generated clients and API tooling will still expose only POST, so this does not actually document the implemented PUT endpoint. Add a real PUT operation, ideally reusing the same request and response definitions.
AGENTS.md reference: specs/AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
| `null` also resets the policy to deny-all. | ||
| - `PUT` behaves identically to `POST`. | ||
| requestBody: | ||
| required: true |
There was a problem hiding this comment.
The new operation says that an empty body or JSON null resets the policy, but requestBody.required: true makes omission invalid and the referenced NetworkPolicy is an object-only schema that rejects null. Generated clients will therefore require an object argument and cannot represent two reset forms accepted by policy_server.go; make the body optional and the schema nullable so the contract matches the runtime.
AGENTS.md reference: specs/AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
|
|
||
| ## `[tenants]` | ||
|
|
||
| Optional multi-tenant mode. When the table is present, tenant resolution is enabled and API key checks apply per tenant instead of globally. Provider types: **`file`** (reads a `tenants.toml` at the path given by `SANDBOX_TENANTS_CONFIG_PATH`, default `~/.opensandbox/tenants.toml`) or **`http`** (fetches tenants from a remote endpoint with in-process caching). |
There was a problem hiding this comment.
This newly added configuration section presents [tenants] as generally optional but omits both startup prerequisites enforced by validate_tenant_config: Docker runtime is rejected because tenant isolation requires Kubernetes namespaces, and a non-empty server.api_key must be removed because it conflicts with tenant-managed keys. Operators following this section with either otherwise-valid setting will receive a startup ValueError; state both constraints alongside the table.
AGENTS.md reference: server/AGENTS.md:L40-L44
Useful? React with 👍 / 👎.
1 participant
Summary
Systematic cleanup of documentation drift, inaccurate/confusing source comments, and noise comments in unit tests across the monorepo (134 files, 5 commits). Pure comment/docs changes plus two contract-level doc corrections — no behavior changes except a log message wording and an inert function removal.
Highlights
Contract corrections (specs, no wire-format changes)
sandbox-lifecycle.yml: endpoint URL format (/port/→ actual/proxy/{port}shapes), creation is asynchronous (202+Pending),NetworkPolicyempty body resets to deny-all (was described as allow-all), IP/CIDR egress targets now supportedexecd-api.yaml:/command/{id}/logscursor is a byte offset (file seek), not a line index;GET /code/contextslanguageis optional; removed never-emitteddestroyedsession status; documented optional--access-tokenenforcementegress-api.yaml: documentedPOST/PUT /policy(implemented but missing from spec), deny-all reset semantics, IP/CIDR target support indns+nftmodeFunctional doc fix
status.updated+UPDATED/AGEprintcolumns (Helm installs silently dropped the field) — synced withconfig/crd/basesDocumentation drift (80+ files)
[tenants]/[ingress].secure_accessdocs, wrong example-config comments (allowed_host_pathsempty = reject bind mounts), outdated DEVELOPMENT.md endpoint/function referencesv1.0.9), missing isolation metrics,HOST_IPmetrics-export fallbackcleanup.shcredential-vaultcommand group + bundled skill, ghostosb sbalias comment,command runoutput-format helpPortsremoved from Go/C# examplesOPENCLAW_SERVER→OPEN_SANDBOX_SERVER), desktopVNC_PASSWORDis required, chrome entrypoint guidance, playwrightTARGET_URLwiringComment cleanup
Verification
docs:build(VitePress): zero errorsruffclean,pytest792 passed (non-k8s) + 563 passed (tests/k8s)ruffclean, help/skills/commands tests passedgofmt -l: no output on any modified filePre-existing environment failure unrelated to this change: execd
pkg/isolationTestMergedViewrequires bwrap privileges (fails on clean checkout too).Intentionally NOT changed (breaking or needs decision): JS SDK
interrupt(contextId)rename, adding execd/proxyendpoint to spec,safego.InitPanicLoggerunused param, cross-languageCredentialMatch.portsdeprecation semantics.